Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…esponsive sidebar, breadcrumb dropdowns, recent items, and file upload integration Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
…cuts, and file upload Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
|
@copilot 浏览器中预览界面显示空白。 |
The useEffect for tracking recent items had unstable array references (cleanParts, allObjects) in its dependency array that changed on every render, causing React error #185 (maximum update depth exceeded). Fix: compute derived values inside the effect body, depend only on location.pathname and the stable addRecentItem callback. Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Root cause: the Fixed in Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
There was a problem hiding this comment.
Pull request overview
This PR implements Phase 2 UX enhancements for the console application, adding skeleton loading states, toast notifications, keyboard shortcuts, responsive sidebar behavior, breadcrumb navigation dropdowns, recent items tracking, and file upload capabilities. The PR also addresses a critical infinite render loop bug (React error #185) by fixing unstable array references in useEffect dependency arrays.
Changes:
- Added skeleton loading components (Grid, Dashboard, Detail) for improved perceived performance
- Integrated sonner toast library for CRUD operation feedback
- Implemented keyboard shortcuts dialog and responsive sidebar auto-collapse
- Extended DataSource interface with file upload methods and FileUploadResult type
- Added recent items tracking with localStorage persistence and sidebar display
- Enhanced breadcrumb navigation with quick-navigation dropdowns for objects
Reviewed changes
Copilot reviewed 24 out of 25 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Added sonner@^2.0.7 dependency for toast notifications |
| packages/types/src/data.ts | Defined FileUploadResult interface and uploadFile/uploadFiles methods on DataSource |
| packages/types/src/index.ts | Exported FileUploadResult type |
| packages/data-objectstack/src/index.ts | Implemented uploadFile and uploadFiles methods with multipart/form-data POST |
| packages/data-objectstack/src/upload.test.ts | Added test coverage for file upload methods |
| apps/console/package.json | Added sonner dependency to console app |
| apps/console/src/components/skeletons/* | Created skeleton loading components for grid, dashboard, and detail views |
| apps/console/src/components/ConsoleToaster.tsx | Configured sonner toaster with theme integration |
| apps/console/src/components/KeyboardShortcutsDialog.tsx | Keyboard shortcuts reference dialog triggered by "?" key |
| apps/console/src/hooks/useResponsiveSidebar.ts | Auto-collapses sidebar on tablet breakpoints (768-1024px) |
| apps/console/src/hooks/useRecentItems.ts | Tracks recent items with localStorage persistence |
| apps/console/src/hooks/useObjectActions.ts | Added toast notifications for delete operations |
| apps/console/src/components/AppHeader.tsx | Enhanced breadcrumbs with dropdown navigation for sibling objects |
| apps/console/src/components/AppSidebar.tsx | Integrated recent items display with Clock icon |
| apps/console/src/components/ConsoleLayout.tsx | Wrapped children with responsive sidebar hook |
| apps/console/src/components/RecordDetailView.tsx | Added skeleton loading state |
| apps/console/src/components/DashboardView.tsx | Added skeleton loading state |
| apps/console/src/App.tsx | Integrated toaster, keyboard shortcuts, and recent items tracking |
| apps/console/src/tests/* | Added tests for skeletons, recent items, and keyboard shortcuts |
| apps/console/src/dataSource.ts | Re-exported FileUploadResult type |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
| const TABLET_MAX = 1024; | ||
|
|
||
| export function useResponsiveSidebar() { | ||
| const { setOpen, isMobile } = useSidebar(); |
There was a problem hiding this comment.
The isMobile variable from useSidebar() is destructured but never used. This suggests it was intended to be part of the logic (e.g., to prevent auto-collapse on mobile) but was forgotten. Consider either using this value in the resize logic or removing it if it's not needed.
| const shortcutGroups: ShortcutGroup[] = [ | ||
| { | ||
| title: 'General', | ||
| shortcuts: [ | ||
| { keys: ['⌘', 'K'], description: 'Open command palette' }, | ||
| { keys: ['?'], description: 'Show keyboard shortcuts' }, | ||
| { keys: ['Esc'], description: 'Close dialog / panel' }, | ||
| ], | ||
| }, | ||
| { | ||
| title: 'Navigation', | ||
| shortcuts: [ | ||
| { keys: ['B'], description: 'Toggle sidebar' }, | ||
| { keys: ['⌘', '/'], description: 'Focus search' }, | ||
| ], | ||
| }, | ||
| { | ||
| title: 'Data Views', | ||
| shortcuts: [ | ||
| { keys: ['N'], description: 'Create new record' }, | ||
| { keys: ['R'], description: 'Refresh data' }, | ||
| { keys: ['⌘', 'E'], description: 'Edit selected record' }, | ||
| ], | ||
| }, | ||
| { | ||
| title: 'Preferences', | ||
| shortcuts: [ | ||
| { keys: ['⌘', 'D'], description: 'Toggle dark mode' }, | ||
| ], | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
The shortcuts use Mac-specific symbols (⌘) without cross-platform adaptation. Windows/Linux users see ⌘ K instead of Ctrl K. Consider detecting the platform and showing appropriate symbols (e.g., using navigator.platform or navigator.userAgentData) to improve accessibility and UX for non-Mac users.
| onProgress?: (percent: number) => void; | ||
| }, | ||
| ): Promise<FileUploadResult> { | ||
| await this.connect(); | ||
|
|
There was a problem hiding this comment.
The upload methods don't validate file size, type, or content before upload. While server-side validation is essential, client-side checks can prevent unnecessary network traffic and provide better UX. Consider adding optional validation parameters (e.g., maxFileSize, allowedMimeTypes) to the upload options to enable client-side checks when needed.
| onProgress?: (percent: number) => void; | |
| }, | |
| ): Promise<FileUploadResult> { | |
| await this.connect(); | |
| onProgress?: (percent: number) => void; | |
| /** | |
| * Optional maximum allowed file size in bytes. | |
| * If provided, the file will be rejected client-side when it exceeds this size. | |
| */ | |
| maxFileSizeBytes?: number; | |
| /** | |
| * Optional list of allowed MIME types. | |
| * If provided, the file will be rejected client-side when its type is not included. | |
| */ | |
| allowedMimeTypes?: string[]; | |
| }, | |
| ): Promise<FileUploadResult> { | |
| await this.connect(); | |
| // Client-side validation to avoid unnecessary uploads when constraints are provided. | |
| if (options?.maxFileSizeBytes != null) { | |
| // Both File and Blob expose the size property in bytes. | |
| if (file.size > options.maxFileSizeBytes) { | |
| throw new ObjectStackError( | |
| `File size exceeds the maximum allowed size of ${options.maxFileSizeBytes} bytes`, | |
| 'UPLOAD_ERROR', | |
| 400, | |
| ); | |
| } | |
| } | |
| if (options?.allowedMimeTypes && options.allowedMimeTypes.length > 0) { | |
| const fileType = (file as File).type ?? ''; | |
| // If the file type is known, enforce the allowed list. | |
| if (fileType && !options.allowedMimeTypes.includes(fileType)) { | |
| throw new ObjectStackError( | |
| `File type "${fileType}" is not allowed`, | |
| 'UPLOAD_ERROR', | |
| 400, | |
| ); | |
| } | |
| } |
| useEffect(() => { | ||
| // Reset loading on navigation; the actual DetailView handles data fetching | ||
| setIsLoading(true); | ||
| queueMicrotask(() => setIsLoading(false)); | ||
| }, [objectName, recordId]); |
There was a problem hiding this comment.
The queueMicrotask pattern here creates a brief loading flicker on every navigation. Since you set isLoading to true and then immediately schedule it to false in a microtask, the skeleton will flash for less than a frame. Consider either: (1) removing the skeleton if the actual component handles its own loading state, or (2) using a minimum display time (e.g., setTimeout with 200ms) to avoid disorienting flickers. The same issue exists in DashboardView.tsx.
| describe('SkeletonGrid', () => { | ||
| it('renders with default props', () => { | ||
| const { container } = render(<SkeletonGrid />); | ||
| const skeletons = container.querySelectorAll('[data-testid="skeleton"]'); | ||
| // Header (5) + 8 rows x 5 cols (40) + toolbar (4) + pagination (4) = 53 | ||
| expect(skeletons.length).toBeGreaterThan(0); | ||
| }); | ||
|
|
||
| it('renders correct number of rows', () => { | ||
| const { container } = render(<SkeletonGrid rows={3} columns={2} />); | ||
| // Should have skeletons for 3 rows x 2 columns in the table body | ||
| const rowContainers = container.querySelectorAll('.border-b'); | ||
| expect(rowContainers.length).toBeGreaterThanOrEqual(3); | ||
| }); | ||
| }); | ||
|
|
||
| describe('SkeletonDashboard', () => { | ||
| it('renders with default props', () => { | ||
| const { container } = render(<SkeletonDashboard />); | ||
| const skeletons = container.querySelectorAll('[data-testid="skeleton"]'); | ||
| expect(skeletons.length).toBeGreaterThan(0); | ||
| }); | ||
|
|
||
| it('renders correct number of widget cards', () => { | ||
| const { container } = render(<SkeletonDashboard cards={3} />); | ||
| // 3 widget cards, each with 3 skeletons + stats row (4 cards x 3 each) + header (2) | ||
| const skeletons = container.querySelectorAll('[data-testid="skeleton"]'); | ||
| expect(skeletons.length).toBeGreaterThan(0); | ||
| }); | ||
| }); | ||
|
|
||
| describe('SkeletonDetail', () => { | ||
| it('renders with default props', () => { | ||
| const { container } = render(<SkeletonDetail />); | ||
| const skeletons = container.querySelectorAll('[data-testid="skeleton"]'); | ||
| expect(skeletons.length).toBeGreaterThan(0); | ||
| }); | ||
|
|
||
| it('renders correct number of field rows', () => { | ||
| const { container } = render(<SkeletonDetail fields={4} columns={1} />); | ||
| const skeletons = container.querySelectorAll('[data-testid="skeleton"]'); | ||
| expect(skeletons.length).toBeGreaterThan(0); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The test assertions are too weak. All tests only check toBeGreaterThan(0) without verifying the actual count based on the props. For example, the test on line 26-31 sets rows={3} columns={2} but only checks that there are at least 3 .border-b elements, not that the skeleton correctly renders 3 rows with 2 columns each. Consider asserting exact counts or at least meaningful ranges to catch rendering bugs.
| const url = `${this.baseUrl}/api/data/${encodeURIComponent(resource)}/upload`; | ||
|
|
||
| const response = await fetch(url, { | ||
| method: 'POST', | ||
| body: formData, | ||
| headers: { | ||
| ...(this.getAuthHeaders()), | ||
| }, | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| const error = await response.json().catch(() => ({ message: response.statusText })); | ||
| throw new ObjectStackError( | ||
| error.message || `Upload failed with status ${response.status}`, | ||
| 'UPLOAD_ERROR', | ||
| response.status, | ||
| ); | ||
| } | ||
|
|
||
| return response.json(); | ||
| } | ||
|
|
||
| /** | ||
| * Upload multiple files to a resource. | ||
| * Posts all files as a single multipart/form-data request. | ||
| * | ||
| * @param resource - The resource/object name to attach the files to | ||
| * @param files - Array of File objects or Blobs to upload | ||
| * @param options - Additional upload options | ||
| * @returns Promise resolving to array of upload results | ||
| */ | ||
| async uploadFiles( | ||
| resource: string, | ||
| files: (File | Blob)[], | ||
| options?: { | ||
| recordId?: string; | ||
| fieldName?: string; | ||
| metadata?: Record<string, unknown>; | ||
| onProgress?: (percent: number) => void; | ||
| }, | ||
| ): Promise<FileUploadResult[]> { | ||
| await this.connect(); | ||
|
|
||
| const formData = new FormData(); | ||
| files.forEach((file, idx) => { | ||
| formData.append(`files`, file, (file as File).name || `file-${idx}`); | ||
| }); | ||
|
|
||
| if (options?.recordId) { | ||
| formData.append('recordId', options.recordId); | ||
| } | ||
| if (options?.fieldName) { | ||
| formData.append('fieldName', options.fieldName); | ||
| } | ||
| if (options?.metadata) { | ||
| formData.append('metadata', JSON.stringify(options.metadata)); | ||
| } | ||
|
|
||
| const url = `${this.baseUrl}/api/data/${encodeURIComponent(resource)}/upload`; | ||
|
|
||
| const response = await fetch(url, { | ||
| method: 'POST', | ||
| body: formData, | ||
| headers: { | ||
| ...(this.getAuthHeaders()), | ||
| }, | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| const error = await response.json().catch(() => ({ message: response.statusText })); | ||
| throw new ObjectStackError( | ||
| error.message || `Upload failed with status ${response.status}`, | ||
| 'UPLOAD_ERROR', | ||
| response.status, | ||
| ); | ||
| } | ||
|
|
||
| return response.json(); | ||
| } |
There was a problem hiding this comment.
Both uploadFile and uploadFiles use the same endpoint /api/data/{resource}/upload. The server needs to distinguish between single and multiple file uploads, which is typically done via different field names or endpoints. Verify that the server expects both requests at the same URL and can differentiate between a single file field named 'file' vs. multiple files named 'files'.
| export function App() { | ||
| return ( | ||
| <ThemeProvider defaultTheme="system" storageKey="object-ui-theme"> | ||
| <ConsoleToaster position="bottom-right" /> |
There was a problem hiding this comment.
The toast position is hardcoded to "bottom-right" in App.tsx (line 350). Consider making this configurable via the ThemeProvider or user preferences, as some users may prefer different positions (top-right, top-center, etc.) based on their workflow or accessibility needs.
| export function App() { | |
| return ( | |
| <ThemeProvider defaultTheme="system" storageKey="object-ui-theme"> | |
| <ConsoleToaster position="bottom-right" /> | |
| const consoleToastPosition = (appConfig as any)?.toastPosition ?? 'bottom-right'; | |
| export function App() { | |
| return ( | |
| <ThemeProvider defaultTheme="system" storageKey="object-ui-theme"> | |
| <ConsoleToaster position={consoleToastPosition} /> |
| type: 'report', | ||
| }); | ||
| } | ||
| }, [location.pathname, addRecentItem]); // eslint-disable-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
The eslint-disable comment may be masking a real issue. The effect uses activeApp (line 130, 136) which is derived from the appName route parameter (line 57), but activeApp is not in the dependency array. If the app changes (e.g., navigating from /apps/crm to /apps/sales), the effect won't re-run because only location.pathname and addRecentItem are dependencies. Consider either adding activeApp to the dependency array or using appName from the location directly inside the effect.
| async uploadFile( | ||
| resource: string, | ||
| file: File | Blob, | ||
| options?: { | ||
| recordId?: string; | ||
| fieldName?: string; | ||
| metadata?: Record<string, unknown>; | ||
| onProgress?: (percent: number) => void; | ||
| }, | ||
| ): Promise<FileUploadResult> { | ||
| await this.connect(); | ||
|
|
||
| const formData = new FormData(); | ||
| formData.append('file', file); | ||
|
|
||
| if (options?.recordId) { | ||
| formData.append('recordId', options.recordId); | ||
| } | ||
| if (options?.fieldName) { | ||
| formData.append('fieldName', options.fieldName); | ||
| } | ||
| if (options?.metadata) { | ||
| formData.append('metadata', JSON.stringify(options.metadata)); | ||
| } | ||
|
|
||
| const url = `${this.baseUrl}/api/data/${encodeURIComponent(resource)}/upload`; | ||
|
|
||
| const response = await fetch(url, { | ||
| method: 'POST', | ||
| body: formData, | ||
| headers: { | ||
| ...(this.getAuthHeaders()), | ||
| }, | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| const error = await response.json().catch(() => ({ message: response.statusText })); | ||
| throw new ObjectStackError( | ||
| error.message || `Upload failed with status ${response.status}`, | ||
| 'UPLOAD_ERROR', | ||
| response.status, | ||
| ); | ||
| } | ||
|
|
||
| return response.json(); | ||
| } |
There was a problem hiding this comment.
The onProgress callback is defined in the upload options but never invoked. If progress tracking is not needed yet, consider removing this option from the interface to avoid confusion. If it is needed, you should use XMLHttpRequest instead of fetch to support progress events, or document that this callback is not yet implemented.
| const shortcutGroups: ShortcutGroup[] = [ | ||
| { | ||
| title: 'General', | ||
| shortcuts: [ | ||
| { keys: ['⌘', 'K'], description: 'Open command palette' }, | ||
| { keys: ['?'], description: 'Show keyboard shortcuts' }, | ||
| { keys: ['Esc'], description: 'Close dialog / panel' }, | ||
| ], | ||
| }, | ||
| { | ||
| title: 'Navigation', | ||
| shortcuts: [ | ||
| { keys: ['B'], description: 'Toggle sidebar' }, | ||
| { keys: ['⌘', '/'], description: 'Focus search' }, | ||
| ], | ||
| }, | ||
| { | ||
| title: 'Data Views', | ||
| shortcuts: [ | ||
| { keys: ['N'], description: 'Create new record' }, | ||
| { keys: ['R'], description: 'Refresh data' }, | ||
| { keys: ['⌘', 'E'], description: 'Edit selected record' }, | ||
| ], | ||
| }, | ||
| { | ||
| title: 'Preferences', | ||
| shortcuts: [ | ||
| { keys: ['⌘', 'D'], description: 'Toggle dark mode' }, | ||
| ], | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
The keyboard shortcuts listed here (e.g., 'Toggle sidebar' with B, 'Create new record' with N, etc.) appear to be documentation-only — there's no implementation of these shortcuts visible in the codebase. Users who open this dialog will expect these shortcuts to work. Either implement these shortcuts or mark them as "Coming soon" / "Planned" to avoid confusion.

Fixed: Blank page in browser preview
The
useEffectfor tracking recent items had unstable array references (cleanPartsfrom.filter(),allObjectsfrom|| []) in its dependency array. These created new array objects on every render, causing the effect to fire →addRecentItem→ state update → re-render → infinite loop (React error #185).Fix: Compute derived values inside the effect body; depend only on
location.pathnameand the stableaddRecentItemcallback.✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.